Allows @inacessible in subgraphs #1638
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
pcmanus
force-pushed
the
inaccessible-subgraphs
branch
from
e2085a2
to
1207b89
Compare
internals-js/src/error.ts
Outdated
|
|
||
| const REFERENCED_INACCESSIBLE = makeCodeDefinition( | ||
| 'REFERENCED_INACCESSIBLE', | ||
| 'An element is marked as @inaccessible but is referenced by a non-inacessible elemnent (at least in post-merging of the subgraphs)' |
There was a problem hiding this comment.
"non-inacessible" -> "non-inaccessible"
"elemnent" -> "element"
Also I don't love the "at least in post-merging", not sure exactly how I'd change it.
There was a problem hiding this comment.
Agreed about the "post-merging" parts that it's not super clear. I ended up removing it because 1) I wasn't sure either how to say that better and 2) that error can also be thrown by non-merge related calls to toAPISchema so it's probably not worth trying to be overly precise after all.
| @@ -1402,6 +1406,51 @@ export class Schema { | |||
| specifiedByDirective(schema: Schema): DirectiveDefinition<{url: string}> { | |||
| return this.getBuiltInDirective(schema, 'specifiedBy'); | |||
| } | |||
|
|
|||
| elementByCoordinate(coordinate: string): NamedSchemaElement<any, any, any> | undefined { | |||
There was a problem hiding this comment.
Can we add a comment that links to a spec that defines what a syntactically correct coordinate is?
There was a problem hiding this comment.
Linked to graphql/graphql-spec#794. Not sure there is better place to link yet since it's one of those graphQL spec changes that seems pretty much finalised but not quite published yet.
internals-js/src/definitions.ts
Outdated
| const argName = argStartIdx < 0 ? undefined : coordinate.slice(argStartIdx + 1, coordinate.length - 2); | ||
| const splittedStart = start.split('.'); | ||
| const typeOrDirectiveName = splittedStart[0]; | ||
| const fieldOrEnumName = splittedStart.length > 1 ? splittedStart[1] : undefined; |
There was a problem hiding this comment.
splittedStart.length > 1 ? splittedStart[1] : undefined;
is equivalent to
splittedStart[1]
since accessing an invalid index in an array will return undefined
| @@ -862,6 +868,10 @@ export function setSchemaAsFed2Subgraph(schema: Schema) { | |||
| completeSubgraphSchema(schema); | |||
| } | |||
|
|
|||
| // This is the full @link declaration as added by `asFed2SubgraphDocument`. It's here primarily for uses by tests that print and match | |||
| // subgraph schema to avoid having to update 20+ tests every time we use a new directive or the order of import changes ... | |||
| export const FEDERATION2_LINK_WTH_FULL_IMPORTS = '@link(url: "https://specs.apollo.dev/federation/v2.0", import: ["@key", "@requires", "@provides", "@external", "@tag", "@extends", "@shareable", "@inaccessible"])'; | |||
There was a problem hiding this comment.
Can this be done programmatically by manipulating allFederationDirectives rather than hardcoding?
There was a problem hiding this comment.
It can, but I actually didn't do it intentionally. The main use of this is for test assertions and I'd hate to have some syntax issue go unnoticed because we made the same programming error in the code tested and the code used to test it. In other words, I think there is value in having this laid out in plain-text at least once so we can eyeball any issue, and don't think it's a big deal to have to have to update that one place every so often, it's having to update 20 places that was getting on my nerves.
But I reckon that this is caution on my part that borders on paranoia, so let me know if you're not convinced and would prefer generating it as I don't feel strongly at all.
query-graphs-js/src/querygraph.ts
Outdated
| const builder = new GraphBuilderFromSchema( | ||
| name, | ||
| schema, | ||
| supergraphSchema ? { apiSchema: supergraphSchema?.toAPISchema(), isFed1: isFed1Supergraph(supergraphSchema) } : undefined, |
There was a problem hiding this comment.
? in supergraphSchema?.toAPISchema() is unnecessary
| : []; | ||
| const element = e.extensions['inaccessible_element']; | ||
| // We only point to subgraphs where the element is marked inaccesssible, as other subgraph are not relevant. | ||
| const elementNodes = element && typeof element === 'string' |
There was a problem hiding this comment.
I find the indentation of this one pretty tough. Might be more readable as:
const elementNodes = element && typeof element === 'string'
? sourceASTs(
...this.subgraphsSchema
.map((schema) => schema.elementByCoordinate(element))
.filter((elt) => elt?.hasAppliedDirective(federationMetadata(elt.schema())!.inaccessibleDirective()))
)
: [];
pcmanus
force-pushed
the
inaccessible-subgraphs
branch
from
1207b89
to
055abf3
Compare
pcmanus
force-pushed
the
inaccessible-subgraphs
branch
from
055abf3
to
7257cd2
Compare
|
Side-note: I rebased on main since #1637 has now be merged. |
Currently, this PR is built on top of #1637, so it would make sense to review that latter PR first and only the last commit is truly relevant to this PR.